VPAAMP-28: Merge MonitorLatency and RateCorrectionWorkerThread#1332
VPAAMP-28: Merge MonitorLatency and RateCorrectionWorkerThread#1332srikanthreddybijjam-comcast wants to merge 2 commits into
Conversation
1961af0 to
c852004
Compare
There was a problem hiding this comment.
Pull request overview
This PR (VPAAMP-28) consolidates the legacy live-latency “rate correction worker thread” logic into the unified AampLatencyMonitor flow, updating both production code paths and related unit tests to use EnableLatencyMonitor() / IsLatencyMonitorEnabled() rather than direct mDisableRateCorrection state.
Changes:
- Remove the dedicated rate-correction thread/members and route enable/disable through
AampLatencyMonitor. - Update stream abstractions (HLS/DASH/core) and playback control paths to toggle latency monitoring instead of mutating
mDisableRateCorrection. - Adjust unit tests and fakes to reflect the new latency-monitor interface.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/tests/PrivateInstanceAAMP/PauseOnPlaybackTests.cpp | Updates tests to use EnableLatencyMonitor() / IsLatencyMonitorEnabled() expectations. |
| test/utests/tests/PrivAampTests/PrivAampTests.cpp | Removes worker-thread tests and updates tune-related assertions to latency-monitor API. |
| test/utests/fakes/FakePrivateInstanceAAMP.cpp | Removes legacy rate-correction stubs and introduces latency-monitor API stubs (currently incomplete). |
| streamabstraction.cpp | Switches re-enable points from mDisableRateCorrection=false to EnableLatencyMonitor(true). |
| priv_aamp.h | Removes legacy worker-thread members/APIs and adds IsLatencyMonitorEnabled(). |
| priv_aamp.cpp | Removes worker-thread implementation and routes enable/disable via AampLatencyMonitor. |
| main_aamp.cpp | Replaces direct member toggling with EnableLatencyMonitor(false) on pause-related flow. |
| fragmentcollector_mpd.cpp | Uses EnableLatencyMonitor(true/false) when entering live / refreshing tracks. |
| fragmentcollector_hls.cpp | Uses EnableLatencyMonitor(true/false) when entering live / refreshing tracks. |
| aampgstplayer.cpp | Changes flush behavior from syncing correction-rate variables to disabling latency monitor. |
| AampLatencyMonitor.h | Adds IsRateCorrectionEnabled() accessor for rate-correction enabled state. |
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12714
- In SetPreferredLanguages(), the new condition uses !mLatencyMonitor->IsRunning() to choose between TuneHelper(eTUNETYPE_SEEK) and TuneHelper(eTUNETYPE_SEEKTOLIVE). Previously this decision was based on mDisableRateCorrection (i.e., whether rate correction was disabled due to user pause/seek). The monitor thread can remain running even when rate correction is disabled, so IsRunning() is not equivalent and may cause unintended seeks to live. Use the rate-correction enabled state instead (e.g., IsLatencyMonitorEnabled()/IsRateCorrectionEnabled()) if the goal is to preserve the prior behavior.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
76acdd3 to
7fbbd1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12712
- This retune path used to branch based on whether rate correction was disabled (
mDisableRateCorrection). The new check uses!mLatencyMonitor->IsRunning(), which is not equivalent: the monitor can be running while correction is disabled via EnableLatencyMonitor(false). In that case this will incorrectly choose SEEKTOLIVE instead of SEEK. Use the rate-correction enabled flag (e.g., IsLatencyMonitorEnabled() / mLatencyMonitor->IsRateCorrectionEnabled()) for this decision, not the thread running state.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
e6239c1 to
332ba2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12720
- This TuneHelper selection now uses
!mLatencyMonitor->IsRunning()to decide betweeneTUNETYPE_SEEKvseTUNETYPE_SEEKTOLIVE. That is not equivalent to the previousmDisableRateCorrectioncheck: the latency monitor can remain running while rate correction is temporarily disabled (e.g., during seek/pause), and the player may also not be at the live point. With the current condition, a language/track retune can incorrectly force a seek-to-live when the user is behind live but the monitor thread is still running. Use the rate-correction enabled flag (e.g.,IsLatencyMonitorEnabled()/mLatencyMonitor->IsRateCorrectionEnabled()) and/orIsAtLivePoint()to preserve the prior behavior.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
332ba2f to
b00acc1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/utests/tests/AampLatencyMonitorTests/AampLatencyMonitorTestCases.cpp:776
- There are a few trailing blank lines at the end of the file after the final test case; please remove the extra empty lines to keep the test file tidy and avoid noise in future diffs.
b00acc1 to
6a5d964
Compare
| TEST_F(AampLatencyMonitorTest, Config_EnableLiveLatencyCorrection_True_EnablesMonitor) | ||
| { | ||
| // Set eAAMPConfig_EnableLiveLatencyCorrection = true. | ||
| ON_CALL(*mMockConfig, IsConfigSet(eAAMPConfig_EnableLiveLatencyCorrection)) | ||
| .WillByDefault(Return(true)); | ||
| ASSERT_TRUE(mConfig->IsConfigSet(eAAMPConfig_EnableLiveLatencyCorrection)); | ||
|
|
||
| // When the flag is true, StartLatencyMonitor() calls Start(). | ||
| mMonitor->Start(MakeFastConfig()); | ||
|
|
||
| ASSERT_TRUE(WaitForRunning()); | ||
| EXPECT_TRUE(mMonitor->IsRunning()); | ||
| EXPECT_TRUE(mMonitor->IsRateCorrectionEnabled()); | ||
| } | ||
|
|
||
| /** | ||
| * @test Config_EnableLiveLatencyCorrection_False_MonitorNotStarted | ||
| * @brief When eAAMPConfig_EnableLiveLatencyCorrection is false (the default) | ||
| * StartLatencyMonitor() must not start the monitor, so IsRunning() | ||
| * must remain false and rate correction must be inactive. | ||
| */ | ||
| TEST_F(AampLatencyMonitorTest, Config_EnableLiveLatencyCorrection_False_MonitorNotStarted) | ||
| { | ||
| // Set eAAMPConfig_EnableLiveLatencyCorrection = false. | ||
| ON_CALL(*mMockConfig, IsConfigSet(eAAMPConfig_EnableLiveLatencyCorrection)) | ||
| .WillByDefault(Return(false)); | ||
| ASSERT_FALSE(mConfig->IsConfigSet(eAAMPConfig_EnableLiveLatencyCorrection)); | ||
|
|
||
| // When the flag is false, StartLatencyMonitor() does NOT call Start(). | ||
| EXPECT_FALSE(mMonitor->IsRunning()); | ||
| mMonitor->EnableRateCorrection(false); | ||
| EXPECT_FALSE(mMonitor->IsRateCorrectionEnabled()); |
There was a problem hiding this comment.
These two tests claim to validate eAAMPConfig_EnableLiveLatencyCorrection controlling whether StartLatencyMonitor() starts the monitor, but they never call PrivateInstanceAAMP::StartLatencyMonitor() (they call mMonitor->Start() directly, and the false-case never attempts a start). As written, the tests will pass regardless of the config-gating logic in PrivateInstanceAAMP. Update the tests to exercise the real integration point (StartLatencyMonitor) or rename/re-scope them so they only test AampLatencyMonitor’s own behavior.
6a5d964 to
00ffe64
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
priv_aamp.cpp:12792
- In SetPreferredLanguages(), the branch deciding between TuneHelper(eTUNETYPE_SEEK) vs TuneHelper(eTUNETYPE_SEEKTOLIVE) now checks
!mLatencyMonitor->IsRunning(). This is not equivalent to the priormDisableRateCorrectionbehavior: the latency monitor can be running while rate correction is intentionally disabled (EnableLatencyMonitor(false) makes the worker wait), which would incorrectly force the SEEKTOLIVE path. Consider using the new rate-correction flag (e.g., IsLatencyMonitorEnabled()/mLatencyMonitor->IsRateCorrectionEnabled()) or an explicit “user is at live” condition instead of IsRunning().
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
priv_aamp.cpp:5486
- TeardownStream() no longer disables/suppresses latency rate correction before stopping/deleting the StreamAbstraction and interacting with the StreamSink. Since AampLatencyMonitor runs on its own thread and can call SetPlayBackRate() concurrently, leaving it enabled during teardown risks racing with sink/stream shutdown (and can apply correction rates during teardown/retune). Consider disabling (EnableLatencyMonitor(false)) or stopping the latency monitor at the start of TeardownStream(), and re-enabling/restarting it after the new tune reaches a stable live/playing state.
//reset discontinuity related flags
ResetDiscontinuityInTracks();
UnblockWaitForDiscontinuityProcessToComplete();
ResetTrackDiscontinuityIgnoredStatus();
lock.unlock();
{
// Using StreamLock to make sure this is not interfering with GetFile() from PreCachePlaylistDownloadTask
std::lock_guard<std::recursive_mutex> streamLock(mStreamLock);
if (mpStreamAbstractionAAMP)
| */ | ||
| TEST_F(AampLatencyMonitorTest, Config_EnableLiveLatencyCorrection_True_EnablesMonitor) | ||
| { | ||
| // Set eAAMPConfig_EnableLiveLatencyCorrection = true. |
There was a problem hiding this comment.
AampLatencyMonitor doesn't depend on AampConfig. These should be added as a test for BuildLatencyConfig only
| } | ||
|
|
||
| // =========================================================================== | ||
| // 6. Non-LLD liveLatencyCorrection (liveOffset-based thresholds) |
There was a problem hiding this comment.
Whats so special about this compared to the LLD test? The functionality has been already validated there.
| if((eTUNETYPE_SEEK == tuneType) || (eTUNETYPE_NEW_SEEK == tuneType)) | ||
| { | ||
| /** Enabled rate Correction by default, seek case and live added later point **/ | ||
| /** Disable rate correction during seek; it will be re-enabled when back at live point **/ |
There was a problem hiding this comment.
This entire block should be deprecated. Should stick with out latency monitor is enabled/disabled using StartLatencyMonitor and teardownStream() -> EnableLatencyMonitor(false)
| NotifyCachedAudioFragmentAvailable(); | ||
| loadNewAudio = false; | ||
| aamp->mDisableRateCorrection = false; | ||
| aamp->EnableLatencyMonitor(true); |
There was a problem hiding this comment.
Again, blindly turning on LatencyMonitor can cause spurious wakeups of latency monitor
There was a problem hiding this comment.
already added in the fragmentcollector handle the re-enabling with all the live-point guards
00ffe64 to
11004bc
Compare
1f7de40 to
086d784
Compare
| void PrivateInstanceAAMP::EnableLatencyMonitor(bool enabled) | ||
| { | ||
| if (g_mockPrivateInstanceAAMP != nullptr) | ||
| { | ||
| g_mockPrivateInstanceAAMP->EnableLatencyMonitor(enabled); | ||
| } | ||
| } | ||
|
|
||
| bool PrivateInstanceAAMP::IsLatencyMonitorEnabled() const | ||
| { | ||
| if (g_mockPrivateInstanceAAMP != nullptr) | ||
| { | ||
| return g_mockPrivateInstanceAAMP->IsLatencyMonitorEnabled(); | ||
| } | ||
| return true; // default: rate correction enabled | ||
| } |
| bool WasRateCorrectionEnabled = mLatencyMonitor->IsRateCorrectionEnabled(); | ||
| if(WasRateCorrectionEnabled) | ||
| { | ||
| AAMPLOG_WARN("DisableRateCorrectionTimeInSeconds : %d isRateCorrectionEnabled : %d", disableRateCorrectionTimeInSeconds, isRateCorrectionEnabled); | ||
| EnableLatencyMonitor(false); | ||
| } |
0597a3c to
12cebd6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
aampgstplayer.cpp:1072
- Flush() still resets the pipeline state, but the previous correction-rate reset was removed without synchronizing the new AampLatencyMonitor state. After a flush, the monitor may keep a stale non-normal mCurrentRate and therefore fail to reapply rate correction when latency still requires it; please sync the monitor’s current rate when the flush succeeds.
bool ret = playerInstance->Flush(position, rate, shouldTearDown, isAppSeek);
if(ret)
{
for (int i = 0; i < AAMP_TRACK_COUNT; i++)
{
//reset buffer control states prior to gstreamer flush so that the first needs_data event is caught
privateContext->mBufferControl[i].flush();
}
| bool WasRateCorrectionEnabled = mLatencyMonitor->IsRateCorrectionEnabled(); | ||
| if(WasRateCorrectionEnabled) | ||
| { | ||
| AAMPLOG_WARN("DisableRateCorrectionTimeInSeconds : %d isRateCorrectionEnabled : %d", disableRateCorrectionTimeInSeconds, isRateCorrectionEnabled); | ||
| EnableLatencyMonitor(false); |
| /** | ||
| * @brief Returns true if rate correction is currently enabled. | ||
| * Thread-safe (atomic load). | ||
| */ | ||
| bool IsRateCorrectionEnabled() const { return mCorrectionEnabled.load(); } |
| @@ -309,7 +309,6 @@ typedef enum | |||
| eAAMPConfig_TimeBasedBufferSeconds, | |||
| eAAMPConfig_MaxDownloadBuffer, /**< Max download buffer in seconds, this can be used to limit player download job scheduling for DASH*/ | |||
| eAAMPConfig_TelemetryInterval, /**< time interval for the telemetry reporting*/ | |||
| void PrivateInstanceAAMP::EnableLatencyMonitor(bool enabled) | ||
| { | ||
| } No newline at end of file | ||
| if (g_mockPrivateInstanceAAMP != nullptr) | ||
| { | ||
| g_mockPrivateInstanceAAMP->EnableLatencyMonitor(enabled); |
| @@ -950,12 +950,6 @@ void AAMPGstPlayer::FlushTrack(AampMediaType type,double pos) | |||
| double audioDelta = aamp->mAudioDelta; | |||
| double subDelta = aamp->mSubtitleDelta; | |||
| double rate = playerInstance->FlushTrack(mediaType, pos, audioDelta, subDelta); | |||
6fbf55c to
f74afbf
Compare
Reason for change: Merged the new flow Test Procedure: Refer jira ticket VPAAMP-28 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>
Reason for change: Merged the new flow Test Procedure: Refer jira ticket VPAAMP-28 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>
f74afbf to
0ebb125
Compare
Reason for change: Merged the new flow
Test Procedure: Refer jira ticket VPAAMP-28
Priority: P2